Should not depend on deployment on root context #1202
Conversation
|
Hi, thanks for your contribution! |
|
I already did, Jenkins. |
|
@guusdk These are automated messages that are currently doesn't track the signing and need a manual whitelisting your account. |
This commit improves support for exposure of the web application on an URL that is not the 'root context' of a domain. In other words, with this commit, one should be able to expose the application not only via an URL like http://meet.example.org, but also via http://example.org/meet This commit fixes: - the URL prefix on the "enter room container" in the welcome page. - determination of what is the room name. - redirection back to the welcome page after hangup. Instead of assuming that a room name is the entire URL path, the code now re-uses the regular expression as documented to be used in NGINX (for forwarding 'room' HTTP requests back to the index.html file). Although not a complete fix (a config can define a custom roomnode function), this is an improvement over 'everything after the slash'.
|
@guusdk Thank you for the PR! I'm a little bit concerned about the way you are detecting and constructing the URLs with regular expressions. I think this way we can easily miss some use cases that are breaking the logic or if we change the allowed chars in future or the format of the URL we should take care of this code too. Also I noticed that the code is not working for root context with more than one directories (for example http://example.org/dir1/dir2/meet/). I think the following solution will be a little bit simpler:
WDYT? |
| @@ -1,9 +1,9 @@ | |||
| <title>Jitsi Meet</title> | |||
| <meta property="og:title" content="Jitsi Meet"/> | |||
| <meta property="og:image" content="/images/jitsilogo.png?v=1"/> | |||
There was a problem hiding this comment.
@guusdk I think you should fix this by setting:
<head>
<base href="http://example.org/meet/">
</head>
There was a problem hiding this comment.
I typically try to avoid the base tag, as in the past, I've ran into subtle issues that were time-consuming to fix (in relation to in-page anchors, if memory serves).
Additioally, I did not want to add them here, as I dislike how this adds a reference to a specific domain and context (although I believe that the href value of base can be relative, but why would you use base at all then?) Without the base element, you can deploy the source on any domain, with them, you'll have to manually modify the source.
I don't really see an upside to having base. It is handy if you want to port an older code-base to a new context, but in general, it adds only confusion.
There was a problem hiding this comment.
This is deployment specific we use, but basically a way to serve the index from one place and rest of the code from a CDN, you can check that in current meet.
| @@ -120,12 +120,12 @@ | |||
| window.addEventListener( | |||
| 'error', loadErrHandler, true /* capture phase type of listener */); | |||
| </script> | |||
| <script><!--#include virtual="/config.js" --></script><!-- adapt to your needs, i.e. set hosts and bosh path --> | |||
There was a problem hiding this comment.
@guusdk why do you need this change? Can't you handle this in your webserver's configs?
There was a problem hiding this comment.
I am actually not using SSI at all - I cannot process them in my webserver. I assumed, perhaps incorrectly, that making the reference relative to the file makes for a safer inclusion.
There was a problem hiding this comment.
Do you have access to the jetty that you are using, can you add some handlers there? I cannot remember can this be added runtime or handlers are setup on startup of jetty...
|
@hristoterezov Thanks for your feedback. I've replied in-line to your in-line comments. As for the regular expression: I completely agree, it's not ideal at all. I'll see if I can introduce a config option, as you suggested. |
|
With recent changes to the upstream source, this PR can't be mended by simple changes. I'll close this one, and start a fresh PR based on the current state of the project. |
The upstream Jitsi Meet application depends on deployment on the root context of a URL (for example: http://meet.example.org). For OFMeet, it is desirable (needed even) to use a non-root context (for example: http://example.org/ofmeet). The original OFMeet code had custom modifications to the Jitsi Meet application (introducing a query parameter). This commit replaces those modifications with a solution that no longer uses a query parameter. This solution is expected to be accepted upstream, which would make 'our' copy of Jitsi Meet be an unmodified copy of the original again. Specifically, this commit: - rolls back custom modifications - applies changes that have also been supplied in an upsteam PR Upstream PR: jitsi/jitsi-meet#1202
This commit improves support for exposure of the web application on an
URL that is not the 'root context' of a domain. In other words, with
this commit, one should be able to expose the application not only via
an URL like http://meet.example.org, but also via http://example.org/meet
This commit fixes:
Instead of assuming that a room name is the entire URL path, the code
now re-uses the regular expression as documented to be used in NGINX
(for forwarding 'room' HTTP requests back to the index.html file).
Although not a complete fix (a config can define a custom roomnode
function), this is an improvement over 'everything after the slash'.